Indy-ready - rabbitmq #15111
Indy-ready - rabbitmq #15111
Conversation
...n/java/io/opentelemetry/javaagent/instrumentation/rabbitmq/RabbitChannelInstrumentation.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/rabbitmq/RabbitChannelInstrumentation.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/rabbitmq/RabbitChannelInstrumentation.java
Outdated
Show resolved
Hide resolved
| @Advice.Argument(5) byte[] body) { | ||
| Context context = Java8BytecodeBridge.currentContext(); | ||
| Span span = Java8BytecodeBridge.spanFromContext(context); | ||
| AMQP.BasicProperties modifiedProps = props; |
There was a problem hiding this comment.
It would have taken less renaming if you had renamed the method argument to originalProps and here used AMQP.BasicProperties props = originalProps;. I know that with inline advice you can't reassign read only arguments. Just curious did it fail with the original code when props was reassigned?
There was a problem hiding this comment.
I've renamed props to originalProps.
I know that with inline advice you can't reassign read only arguments. Just curious did it fail with the original code when props was reassigned?
I don't test advices that use readonly, always change them first, given the known issues with those for inline advice. However, I just did a quick test and I got this error:
java.lang.IllegalStateException: Cannot define writable field access for com.rabbitmq.client.AMQP$BasicProperties arg2 when using delegation
at net.bytebuddy.asm.Advice$OffsetMapping$ForArgument$Unresolved$Factory.make(Advice.java:1800)
at net.bytebuddy.asm.Advice$Dispatcher$Resolved$AbstractBase.<init>(Advice.java:9101)
at net.bytebuddy.asm.Advice$Dispatcher$Delegating$Resolved.<init>(Advice.java:10852)
at net.bytebuddy.asm.Advice$Dispatcher$Delegating$Resolved$ForMethodEnter.<init>(Advice.java:11295)
at net.bytebuddy.asm.Advice$Dispatcher$Delegating$Resolved$ForMethodEnter$WithDiscardedEnterType.<init>(Advice.java:11482)
at net.bytebuddy.asm.Advice$Dispatcher$Delegating$Resolved$ForMethodEnter.of(Advice.java:11338)
at net.bytebuddy.asm.Advice$Dispatcher$Delegating.asMethodEnter(Advice.java:10782)
at net.bytebuddy.asm.Advice.to(Advice.java:378)
There was a problem hiding this comment.
I tried it out and got a slightly different exception. My understanding is that writing to method parameters is fine with non-inline advice. The advice code runs in a separate class and even when you change the value for the local parameter it isn't reflected in the original method. Where things break is when the same advice is applied as inline advice. There byte-buddy will detect the attempt to change the value of the method parameter and reject it because the parameter is read only.
| // We have to save off the queue name here because it isn't available to the consumer later. | ||
| if (consumer != null && !(consumer instanceof TracedDelegatingConsumer)) { | ||
| consumer = new TracedDelegatingConsumer(queue, consumer, channel.getConnection()); | ||
| Consumer modifiedConsumer = consumer; |
There was a problem hiding this comment.
you don't really need this local, could just return the new value from within the if
There was a problem hiding this comment.
Good point. I've just added the changes.
3 participants
Part of #13031